Skip to content

fix(upgrade): preserve summary after progress cleanup#9860

Merged
jdx merged 3 commits into
jdx:mainfrom
risu729:fix/upgrade-summary-progress-clear
May 14, 2026
Merged

fix(upgrade): preserve summary after progress cleanup#9860
jdx merged 3 commits into
jdx:mainfrom
risu729:fix/upgrade-summary-progress-clear

Conversation

@risu729

@risu729 risu729 commented May 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • finish and reset progress jobs created after install/uninstall work before printing the upgrade summary
  • add an e2e regression test that forces progress UI output and checks that no terminal clear sequence follows the summary row

Root cause

PR #9779 fixed duplicate upgrade progress output by clearing the install progress session after install_all_versions. mise upgrade then creates fresh progress jobs for old-version uninstall cleanup, but those jobs were still registered when the command printed the final Upgraded N tools: summary. On process shutdown, stop_clear() cleared the remaining progress rows from the terminal, which could erase the summary detail lines and leave only the header visible.

Addresses discussion #9856. Likely regression from #9779.

Testing

  • cargo fmt --check
  • git diff --check
  • mise run test:e2e e2e/cli/test_upgrade_progress_summary
  • mise run test:e2e e2e/cli/test_upgrade

This PR was generated by an AI coding assistant.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request ensures that the progress UI is properly terminated before printing the upgrade summary, preventing terminal clear sequences from erasing the output. It refactors MultiProgressReport to expose a finish_progress method and adds a new E2E test to verify the fix. The reviewer suggested moving the finish_progress call earlier in the upgrade process to avoid potential terminal state conflicts with subsequent operations like shim rebuilding or pipx reinstallation.

Comment thread src/cli/upgrade.rs
@greptile-apps

greptile-apps Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a regression from #9779 where mise upgrade's final summary could be erased by the progress UI on process shutdown. The root cause was that fresh progress jobs created during old-version uninstall cleanup were still registered with clx when print_summary ran, so stop_clear() at shutdown could wipe the detail lines.

  • finish_progress() is extracted as a public method on MultiProgressReport, calling progress::stop() then reset_jobs() to render the last state and deregister all jobs from clx.
  • Two finish_progress() calls are added in upgrade.rs: one after the uninstall loop and a defensive second one immediately before print_summary(), covering any new jobs created by rebuild_shims_and_runtime_symlinks or PIPXBackend::reinstall_all.
  • A new e2e regression test sets MISE_FORCE_PROGRESS=1, runs mise up dummy, and verifies that no ANSI erase-in-display sequence (ESC[…J) appears in the output after the summary line.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to progress job lifecycle management and is protected by a targeted regression test.

The fix correctly stops and deregisters progress jobs before printing the upgrade summary, and the double finish_progress() calls are harmless because reset_jobs() leaves clx's job list empty for the second call. The e2e test exercises the exact failure mode described in the root-cause analysis.

No files require special attention.

Important Files Changed

Filename Overview
src/ui/multi_progress_report.rs Extracts finish_progress() as a public method (calling progress::stop() + reset_jobs()), then delegates footer_finish to it — clean refactor, no logic change.
src/cli/upgrade.rs Adds two mpr.finish_progress() calls: one after the uninstall loop and one immediately before print_summary(), ensuring no lingering progress jobs are registered when the summary is written to the terminal.
e2e/cli/test_upgrade_progress_summary New regression test that forces progress UI output and verifies no ANSI erase-in-display escape (ESC[…J) follows the upgrade summary line.

Reviews (2): Last reviewed commit: "test(upgrade): clarify progress summary ..." | Re-trigger Greptile

Comment thread e2e/cli/test_upgrade_progress_summary Outdated
@risu729 risu729 marked this pull request as ready for review May 14, 2026 16:43
@jdx jdx merged commit 159b41f into jdx:main May 14, 2026
32 checks passed
@risu729 risu729 deleted the fix/upgrade-summary-progress-clear branch May 14, 2026 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants